Skip to content

Upstream Trusted Types enforcement in EnsureCSPDoesNotBlockStringCompilation#659

Merged
lukewarlow merged 4 commits intow3c:mainfrom
lukewarlow:upstream-tt
Sep 9, 2024
Merged

Upstream Trusted Types enforcement in EnsureCSPDoesNotBlockStringCompilation#659
lukewarlow merged 4 commits intow3c:mainfrom
lukewarlow:upstream-tt

Conversation

@lukewarlow
Copy link
Copy Markdown
Member

@lukewarlow lukewarlow commented May 14, 2024

Updates EnsureCSPDoesNotBlockStringCompilation to upstream changes from the Trusted Types spec. For non timers this now goes through the motions of checking CSP for trusted types and doing neccessary enforcement.

unsafe-eval is left as is.


Preview | Diff

@lukewarlow
Copy link
Copy Markdown
Member Author

@annevk just so I'm not putting all this spec stuff on your plate, do you know who else might be able to review this?

@lukewarlow
Copy link
Copy Markdown
Member Author

cc @mikewest and @antosart pinging spec editors in case either have time to review this.

@mikewest mikewest requested review from mikewest and removed request for annevk September 4, 2024 15:01
@mikewest
Copy link
Copy Markdown
Member

mikewest commented Sep 4, 2024

I'll take a look tomorrow. 👍

Copy link
Copy Markdown
Member

@mikewest mikewest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The algorithm LGTM. I left a few nits that seem relevant to me, but nothing substantive.

Comment thread index.bs Outdated
Comment thread index.bs Outdated
Comment thread index.bs Outdated
Comment thread index.bs Outdated

1. For each |arg| in |parameterArgs|:

1. Let |index| be the index of |arg| in |parameterArgs|.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest writing this loop differently, as I don't think Infra provides a way to get the index of a given element in a list. Something like the following:

1.  Assert: |parameterArgs|' [list/length=] is equal to [parameterStrings]' [=list/length=].
1.  [=list/iterate|For each=] |index| of [=the range=] 0 to |parameterArgs|' [=list/length=]:
    1.  Let |arg| be |parameterArgs|[|index|].

Alternatively, we could add something to Infra to either create a For each variant that provides both an item and its index, or some mechanism to get the index of a given item? @annevk might have thoughts about which path might be preferable.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've gone with what you suggested for now.

Comment thread index.bs Outdated
@lukewarlow lukewarlow requested a review from mikewest September 9, 2024 11:39
Copy link
Copy Markdown
Member

@mikewest mikewest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Happy to merge this after you either accept or reject my annoying nits. :)

Comment thread index.bs
Comment thread index.bs Outdated
Comment thread index.bs Outdated
@lukewarlow
Copy link
Copy Markdown
Member Author

This should be ready to merge @mikewest (I have permissions but don't know if there's any specific commit message changes you'd like to make)

@mikewest
Copy link
Copy Markdown
Member

mikewest commented Sep 9, 2024

Nope. In that case, still still LGTM. Feel free to merge it. :)

@lukewarlow lukewarlow merged commit ce17e10 into w3c:main Sep 9, 2024
github-actions Bot added a commit that referenced this pull request Sep 9, 2024
…ilation (#659)

SHA: ce17e10
Reason: push, by lukewarlow

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants